GPT Review not-so-smart-contracts/algorand #278

0xPhaze · 2023-04-12T13:09:47Z

No description provided.

S3v3ru5 · 2023-04-14T14:34:00Z

not-so-smart-contracts/algorand/access_controls/README.md

@@ -1,18 +1,17 @@
 # Access Controls

-Lack of appropriate checks for application calls of type UpdateApplication and DeleteApplication allows attackers to update application’s code or delete an application entirely.
+Inadequate checks for application calls of type UpdateApplication and DeleteApplication can allow attackers to alter an application's code or delete it entirely.


We should probably stick with previously used update instead of alter an (?)

+1 , update is more appropriated here

S3v3ru5 · 2023-04-14T14:50:21Z

not-so-smart-contracts/algorand/rekeying/README.md


-Note: From Teal v6, Applications can also be rekeyed by executing an inner transaction with "RekeyTo" field set to a non-zero address. Rekeying an application allows to bypass the application logic and directly transfer Algos and assets of the application account.
+Note: Starting from Teal v6, applications can also be rekeyed by executing an inner transaction with the "RekeyTo" field set to a non-zero address. Rekeying an application allows for bypassing the application logic and directly transferring Algos and assets of the application account.


We should replace

Starting from Teal v6, applications can also be

with

Starting from Teal v6, application accounts can also be

montyly · 2023-04-17T13:39:21Z

not-so-smart-contracts/algorand/access_controls/README.md

@@ -1,18 +1,17 @@
 # Access Controls

-Lack of appropriate checks for application calls of type UpdateApplication and DeleteApplication allows attackers to update application’s code or delete an application entirely.
+Inadequate checks for application calls of type UpdateApplication and DeleteApplication can allow attackers to alter an application's code or delete it entirely.


+1 , update is more appropriated here

montyly · 2023-04-17T13:40:27Z

not-so-smart-contracts/algorand/access_controls/README.md


 ## Exploit Scenarios

-A stateful contract serves as a liquidity pool for a pair of tokens. Users can deposit the tokens to get the liquidity tokens and can get back their funds with rewards through a burn operation. The contract does not enforce restrictions for UpdateApplication type application calls. Attacker updates the approval program with a malicious program that transfers all assets in the pool to the attacker's address.
+A stateful contract functions as a liquidity pool for two tokens. Users can deposit these tokens to acquire liquidity tokens and can retrieve their funds along with rewards via a burn operation. The contract, however, does not enforce any restrictions for UpdateApplication type application calls. An attacker can update the approval program with a malicious program that transfers all the assets in the pool to their address.


a stateful contract functions should be change, as function as a specific meaning.

montyly · 2023-04-17T13:40:53Z

not-so-smart-contracts/algorand/access_controls/README.md


 ## Recommendations

- Set proper access controls and apply various checks before approving applications calls of type UpdateApplication and DeleteApplication.
+- Implement appropriate access controls and conduct various checks before approving application calls of type UpdateApplication and DeleteApplication.


conduct should be reverted I think

montyly · 2023-04-17T13:41:11Z

not-so-smart-contracts/algorand/access_controls/README.md


- Use [Tealer](https://github.com/crytic/tealer) to detect this issue.
+- Utilize [Tealer](https://github.com/crytic/tealer) to identify this issue.


This line should be reverted I think

montyly · 2023-04-17T13:42:13Z

not-so-smart-contracts/algorand/asset_id_check/README.md


-Lack of verification of asset id in the contract allows attackers to transfer a different asset in place of the expected asset and mislead the application.
+Failing to verify the asset ID in a contract can allow attackers to transfer a different asset instead of the expected one, potentially misleading the application.


Lack of verification was better I think.

montyly · 2023-04-17T13:42:58Z

not-so-smart-contracts/algorand/asset_id_check/README.md


 ## Description

-Contracts accepting and doing operations based on the assets transferred to the contract must verify that the transferred asset is the expected asset by checking the asset Id. Absence of check for expected asset Id could allow attackers to manipulate contract’s logic by transferring a fake, less or more valuable asset instead of the correct asset.
+Contracts that accept and perform operations based on assets transferred to them must verify that the transferred asset is indeed the expected asset by checking the asset ID. Neglecting to check the expected asset ID could enable attackers to manipulate the contract's logic by transferring a fake, less valuable, or more valuable asset instead of the correct one.


Neglecting should be reverted

montyly · 2023-04-17T13:47:17Z

not-so-smart-contracts/algorand/denial_of_service/README.md

@@ -1,18 +1,18 @@
 # Denial of Service

-When a contract does not verify whether an account has opted in to an asset and attempts to transfer that asset, an attacker can DoS other users if the contract's operation is to transfer asset to multiple accounts.
+A denial of service (DoS) attack can occur when a contract does not verify whether an account has opted into an asset and then attempts to transfer that asset. If the contract's operation involves transferring an asset to multiple accounts, an attacker can use this weakness to launch a DoS attack against other users.


Note to myself: continue the review from here

GPT Review

3c35ac6

0xPhaze requested review from S3v3ru5 and montyly as code owners April 12, 2023 13:09

Run format

a86c5c3

S3v3ru5 reviewed Apr 14, 2023

View reviewed changes

S3v3ru5 approved these changes Apr 14, 2023

View reviewed changes

montyly reviewed Apr 17, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GPT Review not-so-smart-contracts/algorand #278

GPT Review not-so-smart-contracts/algorand #278

0xPhaze commented Apr 12, 2023

S3v3ru5 Apr 14, 2023

montyly Apr 17, 2023

S3v3ru5 Apr 14, 2023

montyly Apr 17, 2023

montyly Apr 17, 2023

montyly Apr 17, 2023

montyly Apr 17, 2023

montyly Apr 17, 2023

montyly Apr 17, 2023

montyly Apr 17, 2023


		Note: From Teal v6, Applications can also be rekeyed by executing an inner transaction with "RekeyTo" field set to a non-zero address. Rekeying an application allows to bypass the application logic and directly transfer Algos and assets of the application account.
		Note: Starting from Teal v6, applications can also be rekeyed by executing an inner transaction with the "RekeyTo" field set to a non-zero address. Rekeying an application allows for bypassing the application logic and directly transferring Algos and assets of the application account.


		- Use [Tealer](https://github.com/crytic/tealer) to detect this issue.
		- Utilize [Tealer](https://github.com/crytic/tealer) to identify this issue.


		Lack of verification of asset id in the contract allows attackers to transfer a different asset in place of the expected asset and mislead the application.
		Failing to verify the asset ID in a contract can allow attackers to transfer a different asset instead of the expected one, potentially misleading the application.

GPT Review not-so-smart-contracts/algorand #278

Are you sure you want to change the base?

GPT Review not-so-smart-contracts/algorand #278

Conversation

0xPhaze commented Apr 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment